(fix) Controller I/O table: sort Action column by display string #13039
Conversation
ronso0
changed the title
|
This clang-format fix is nonsense: VERIFY_OR_DEBUG_ASSERT(del) {
- return QString();
+ return QString();
}I ignored it locally because I thought it was a false-positive of my version or whatever. |
| case MIDI_COLUMN_ACTION: | ||
| if (role == Qt::UserRole) { | ||
| // TODO(rryan): somehow get the delegate display text? | ||
| return QVariant(mapping.control.group + QStringLiteral(",") + mapping.control.item); | ||
| if (role == Qt::UserRole) { // sort by displaystring | ||
| QStyledItemDelegate* del = getDelegateForIndex(index); | ||
| VERIFY_OR_DEBUG_ASSERT(del) { | ||
| return QString(); | ||
| } | ||
| return del->displayText(QVariant::fromValue(mapping.control), QLocale()); | ||
| } | ||
| return QVariant::fromValue(mapping.control); | ||
| case MIDI_COLUMN_COMMENT: |
There was a problem hiding this comment.
I ignored it locally because I thought it was a false-positive of my version or whatever.
maybe it goes away when you wrap the case block in another pair of braces?
There was a problem hiding this comment.
Ah okay, you think clang expects all returns of a case to be indented identically?
I'll try that, but tbh I'm a bit tired of making clang-format happy with previously unrequired formatting.
There was a problem hiding this comment.
Ah okay, you think clang expects all returns of a case to be indented identically?
Nah, the syntax rules about switch-case is just a bit weird in general, so I just think this is bug in clang format which might be worked around that way.
I'll try that, but tbh I'm a bit tired of making clang-format happy with previously unrequired formatting.
I agree. I mean I wouldn't want to go without clang-format though so I usually just do what makes it happy, even if I don't agree with it.
There was a problem hiding this comment.
Whatever it takes, just make it happy even if its accepting the garbage it produces. Introducing a CI failure deliberately is not worth the potential hassle later.
ronso0
force-pushed
the
controller-io-sort-by-displaytext
branch
from
d8f27b3
to
0c116b7
Compare
|
Merge? |
|
I'm sorry I don't mean to reiterate, but I think we should just accept the bogus clang-format suggestion because I don't want to merge this PR unless its green. Merging a failing PR would probably have annoying effects later (much more than a misformatted line). So can you please apply the pre-commit patch, amend and force push? |
|
Oh, sorry. Will fix it. |
ronso0
force-pushed
the
controller-io-sort-by-displaytext
branch
from
0c116b7
to
5c05c76
Compare
|
@Swiftb0y This looks good now, merge? |
|
friendly pinging @Swiftb0y |
|
sorry for not responding earlier, this got lost among the flood of notifications. lgtm indeed. |
Addresses an old TODO and fixes #13036